Maybe we should use stat.S_IRWUX here instead of a magic number.

Maybe we should use stat.S_IRWUX here instead of a magic number.

That "magic number" is the standard unix permission expression, when you see 700 it's clear that it's: owner (read + write + execute), group (none), public (none). What S_IRWUX means heaven only knows.

I mistyped it. It's S_IRWXU which is for Read Write eXecute User. It's very commonly seen in code written for Linux.

I'm fine with either, but let's be consistent. We're using the constatns from stats in a test, but the magic number elsewhere.

This test isn't asserting anything after create_secure_file(), which makes it unclear what it's testing.

I think it's relying on the fact that if an exception is thrown, the test will fail. In essence, it's testing that no exceptions are thrown. Maybe a comment is in order. # test will fail if any exceptions are thrown or something similar that explains the intent of the test.

I believe if create_secure_file() returns the file handler without closing, you could do something like:

with open(create_secure_file(password_file), "wb"):

This would avoid the os.close(os.open(...)) ugliness in the create_secure_file() function. You should probably then rename it to open_new_secure_file() or something.

What purpose would that serve? We would just be doing something like

with open(create_secure_file(password_file), "wb"):
    pass

?

Plus, I don't mind the os.close(os.open(...)) ugliness if it means that we're maintaining consistency in utils.py (where we're simply creating secure files/directories, not opening them).

There are better ways to improve readability. Creating and opening and then closing the file is not a very intuitive workflow just for creating a file.

You wouldn't use open() and then pass. The code that used it would look like this:

diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py
index abeb34dc..9ec3dc37 100644
--- a/monkey/monkey_island/cc/server_utils/encryptor.py
+++ b/monkey/monkey_island/cc/server_utils/encryptor.py
@@ -6,7 +6,7 @@ import os
 from Crypto import Random  # noqa: DUO133  # nosec: B413
 from Crypto.Cipher import AES  # noqa: DUO133  # nosec: B413
 
-from monkey_island.cc.server_utils.file_utils import create_secure_file
+from monkey_island.cc.server_utils.file_utils import open_new_secure_file
 
 __author__ = "itay.mizeretz"
 
@@ -23,12 +23,11 @@ class Encryptor:
         if os.path.exists(password_file):
             self._load_existing_key(password_file)
         else:
-            create_secure_file(path=password_file)
             self._init_key(password_file)
 
     def _init_key(self, password_file):
         self._cipher_key = Random.new().read(self._BLOCK_SIZE)
-        with open(password_file, "wb") as f:
+        with open(open_new_secure_file(password_file, "wb")) as f:
             f.write(self._cipher_key)
 
     def _load_existing_key(self, password_file):

Rather than close(open()) and then open() the file again later, you would just open the file and write to it. Open a file and write to it is the standard workflow for handling files. Create a directory and use it is the standard workflow for directories. There's not really an inconsistency.

You could consider names like get_file_descriptor_for_secure_file() instead of open_secure_file() or something else. The point is, we should just open a file and write to it.

Why do we even need the complexity of the flags? Why not simply Path(path).touch(0o700)?

Nice; changed it.

Strange, why do we need () here?

That's how black formats it. Because it's in the next line.

That means the comment needs to be above the statement and the statement needs to be one like, like so: file_sharing = win32file.FILE_SHARE_READ

No clue about what win32job.CreateJobObject does, why do we call it here, what argument it specifies etc. The whole win32file.CreateFile function should be called with keyword arguments.

Generally, just looks like a dirty workaround, there should be an easier way to pass Null. Either way, this should be hidden in a clearly named function.

The whole win32file.CreateFile function should be called with keyword arguments.

Can't. It throws an error that it can't be called with keyword arguments.

Generally, just looks like a dirty workaround, there should be an easier way to pass Null.

I agree. Couldn't find an easier way, though. I'll try again, perhaps.

It's OK if that's the only way we can provide NULL argument, but you should create a method called get_null_value_for_win32 or something and call it instead. Then it would be clear that we actually provide NULL as the last argument and not some kind of created job.

Lastly, don't put comments that obviously violate line length inline, put them above the statement.

why do we need this flag?

Using win32file.FILE_ATTRIBUTE_NORMAL throws an access denied error. win32file.FILE_FLAG_BACKUP_SEMANTICS doesn't probably because of:

The operating system ensures that the calling process overrides file security checks, provided it has the necessary permission to do so.

(Source: http://timgolden.me.uk/pywin32-docs/win32file_FILE_FLAG_BACKUP_SEMANTICS.html)